-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨improve Extension status conditions #683
✨improve Extension status conditions #683
Conversation
Type: ocv1alpha1.TypeDeleting, | ||
Status: metav1.ConditionFalse, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is tricky. I think this combo of TypeDeleting
and ConditionFalse
would be interpreted as "this extension is not in the deleting state"
And I suppose that "DeletionFailed" could mean "I'm no longer actively in the deleting state because the deletion attempt failed".
But what happens at this point? Does kapp-controller
keep retry-ing the deletion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Digging into the codebase (haven't verified yet by trying it out), looks like it doesn't. If the deletion fails, the process is still marked as completed (https://github.com/carvel-dev/kapp-controller/blob/d89101eb78b2c26a73a7afd8e8ae3111a6731dde/cli/pkg/kctrl/cmd/app/app_tailer.go#L52) and the watches are closed (https://github.com/carvel-dev/kapp-controller/blob/d89101eb78b2c26a73a7afd8e8ae3111a6731dde/cli/pkg/kctrl/cmd/app/app_tailer.go#L58).
Going a bit further and looking into the PackageInstall reconciler to see what happens - looks like if deletion fails, and the App's status conveys the same - the packageInstall just updates its status: https://github.com/carvel-dev/kapp-controller/blob/d89101eb78b2c26a73a7afd8e8ae3111a6731dde/pkg/packageinstall/packageinstall.go#L408-L428.
This makes sense, because the reconcile period for the App CR is set by the user (and defaulted to 30sec if it's unset).
This is why they seem to have the DeleteFailed
condition which is set to true when Deleting is set to false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can tell, kapp-controller
does seem to keep retrying the App delete, though I'm not sure if there's anything to limit the retries: https://github.com/carvel-dev/kapp-controller/blob/d89101eb78b2c26a73a7afd8e8ae3111a6731dde/pkg/app/app_reconcile.go#L36
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ankitathomas Let's ask the question in #carvel channel upstream for clarification. They would be able to point us to right code.
@@ -77,6 +77,7 @@ const ( | |||
// TODO(user): add more Types, here and into init() | |||
TypeInstalled = "Installed" | |||
TypeResolved = "Resolved" | |||
TypeDeleting = "Deleting" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have a more generic "Progressing" status instead? That way, whether we are in the process of installing, upgrading or deleting, we can set Progressing=True
(and conversely Progressing=False
would mean we're at a steady state).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need a separate condition for this? I think we could achieve the same thing with Installed
, with Installed=True
being our steady state and False
or Unknown
for other states, with the Reason
giving more details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried if this may cause confusion - what would it mean when: Install is either fail/succeeded, with Progressing - True. How does the user know what action is being taken next? We would have to ensure that when Progressing is set to true, then other statuses are moved to Unknown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we can instead re-name this condition to capture just the Delete
action similar to how we handle others.
TypeDelete
Fail - means App'skcv1alpha1.DeleteFailed
is True,kcv1alpha1.Deleting
is false.TypeDelete
Unknown - means App'skcv1alpha1.Deleting
is true or any other combination which we are unsure of what it represents.
Looking into how Kapp is handling their status references - https://github.com/carvel-dev/kapp-controller/blob/d89101eb78b2c26a73a7afd8e8ae3111a6731dde/cli/pkg/kctrl/cmd/app/app.go#L39-L64 - All they are trying to map is Reconcile Fail/Succeeded/Progressing, Delete Progressing/Failed. So I'm hoping that the above should capture what we want.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can try this option if it's ok to have the TypeDelete
condition on every Extension all the time. kapp-controller
only allows one of the Reconciling
/Deleting
condition sets on an App at a time, I wasn't sure if we wanted to switch the way we mandate all defined conditions presence on each Extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried if this may cause confusion - what would it mean when: Install is either fail/succeeded, with Progressing - True. How does the user know what action is being taken next?
We can use the Reason
and Message
to get into more specifics.
Taking a stab at enumerating various states:
State | Progressing | Installed |
Initial install is in progress | True Reason=Installing Message="installing $bundleID" |
False Reason=NothingInstalled |
Initial install is failing with a non-terminal error (i.e. retries might resolve the error in the future) | True Reason=RetryingInstallation Message="retrying installation of $bundleID due to error: $error" |
False Reason=NothingInstalled |
Initial install failed with a terminal error (i.e. retries will never resolve in the future without some external change) | False Reason=InstallFailed Message="failed installation of $bundleID: $error |
False Reason=NothingInstalled |
Install failed, deletion is in progress | True Reason= Uninstalling Message="uninstalling failed installation of $bundleID" |
False Reason=NothingInstalled |
Initial install failed, deletion is failing | True Reason=RetryingUninstall Message="retrying uninstall of $bundleID due to error: $error" |
False Reason=NothingInstalled |
Initial install failed, deletion failed | False Reason=UninstallFailed Message="failed uninstall of $bundleID: $error>" |
False Reason=NothingInstalled |
Install/upgrade succeeded, steady state | False Reason=NothingToDo |
True Reason=InstallSucceeded Message="successfully installed $bundleID" |
Install succeeded, upgrade is in progress | True Reason=Upgrading Message="upgrading to $newBundleID |
True Reason=InstallSucceeded Message="successfully installed $oldBundleID" |
Install succeeded, upgrade is failing with a non-terminal error | True Reason=RetryingUpgrade Message="retrying upgrade of $newBundleID due to error: $error" |
True Reason=InstallSucceeded Message="successfully installed $oldBundleID" |
Install succeeded, upgrade failed with a terminal error | False Reason=UpgradeFailed Message="failed upgrade to $newBundleID: $error" |
True Reason=InstallSucceeded Message="successfully installed $oldBundleID" |
Install succeeded, deletion is in progress | True Reason=Uninstalling Message="uninstalling $bundleID" |
True Reason=InstallSucceeded Message="successfully installed $bundleID" |
Install succeeded, deletion is failing with a non-terminal error | True Reason=RetryingUninstall Message="retrying uninstall of $bundleID due to error: $error" |
True Reason=InstallSucceeded Message="successfully installed $bundleID" |
Install succeeded, deletion is failing with a terminal error | False Reason=UninstallFailed Message="failed uninstall of $bundleID: $error" |
True Reason=InstallSucceeded Message="successfully installed $bundleID" |
Notice how Progressing
and Installed
are actually orthogonal. Installed
does not depend on Progressing
or vice-versa.
We would have to ensure that when Progressing is set to true, then other statuses are moved to Unknown.
I don't think so. My idea and goal here is actually that a Progressing condition gives us a place to keep some status about what we're doing so that we don't have to wipe out some status about what we did before.
For example, if someone installs v1, and then tries to upgrade to v2, but v2 fails the permissions preflight check, the actual state of the system is:
- v1 is still doing its thing untouched
- v2 failed the upgrade preflight check (which might resolve in the future if the admin reconfigures the permissions of the Extension/App's service account).
It would be really nice to be able to say "v1 is still installed and running, but here's why we can't upgrade to v2."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't a progressing/failing update/delete possibly impact the currently installed extension? If the idea is to have a separate condition specific to preflight checks, we may have to add more support for that separately from the kapp-controller
App conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might impact it, yes. Hopefully App
's status would tell us enough about the failure to help us know if the current installation was impacted or not.
This is probably a good discussion topic with the Carvel folks.
4b5bbf4
to
c069984
Compare
c069984
to
b24b20d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #683 +/- ##
==========================================
- Coverage 68.15% 67.41% -0.75%
==========================================
Files 22 22
Lines 1429 1470 +41
==========================================
+ Hits 974 991 +17
- Misses 390 414 +24
Partials 65 65
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
4ef9059
to
3157a50
Compare
3157a50
to
03904d9
Compare
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Status: metav1.ConditionFalse, | ||
Reason: ocv1alpha1.ReasonResolutionFailed, | ||
Reason: ocv1alpha1.ReasonPending, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're not progressing, doesn't that mean we're at steady state with nothing to do? Pending
seems like it would indicate that we are progressing and waiting on some action to occur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree to that. ProgressingFalse
can indicate a few things:
- Installation of the bundle was successful, we are in a stable state.
- Installation of bundle failed, we are again in a stable state, but look to the install status to know more.
- We are at the Resolution step for the next reconcile probably caused by upgrade - haven't yet reached to the point of install.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. If it is assumed that when Progressing
is False
we are in a stable state maybe Stable
as the reason is sufficient?
|
||
mediaType, err := bundle.MediaType() | ||
if err != nil { | ||
setInstalledStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) | ||
if ext.Status.InstalledBundleResource == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't read our own status here to determine if there is an installed bundle. Instead, we need to Get
the App
that would be managed by this Extension
.
If it does not exist:
- installedBundleResource = ""
- installed condition = False
If we encountered some other error:
- don't touch installedBundleResource?
- installed condition = Unknown
If it does exist:
- installedBundleResource =
- installed condition = true
setDeprecationStatusesUnknown(&ext.Status.Conditions, "kapp apis are unavailable", ext.GetGeneration()) | ||
return ctrl.Result{}, errkappAPIUnavailable | ||
} | ||
|
||
// TODO: Improve the resolution logic. | ||
bundle, err := r.resolve(ctx, *ext) | ||
if err != nil { | ||
ext.Status.InstalledBundleResource = "" | ||
setInstalledStatusConditionUnknown(&ext.Status.Conditions, "installation has not been attempted as resolution failed", ext.GetGeneration()) | ||
if ext.Status.InstalledBundleResource == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here about not reading our own status.
ext.Status.ResolvedBundleResource = "" | ||
setResolvedStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) | ||
setProgressingStatusConditionResolutionFailed(&ext.Status.Conditions, "resolution failed: "+err.Error(), ext.GetGeneration()) | ||
setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as resolution failed", ext.GetGeneration()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you didn't touch this part of the code, but we should be able to determine deprecation status:
- for package and channel if we have a catalog
- for bundle if we have an installed bundle
Can you make a follow-up issue to improve this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#671 should address it
// hasn't been attempted yet, due to the spec being invalid. | ||
setInstalledStatusConditionUnknown(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) | ||
} | ||
setProgressingStatusConditionUnsupportedOrInvalidBundle(&ext.Status.Conditions, "failed to read bundle mediaType: "+err.Error(), ext.GetGeneration()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is generally prefered to use fmt.Sprintf
over string concatenation via +
.
setProgressingStatusConditionUnsupportedOrInvalidBundle(&ext.Status.Conditions, "failed to read bundle mediaType: "+err.Error(), ext.GetGeneration()) | |
setProgressingStatusConditionUnsupportedOrInvalidBundle(&ext.Status.Conditions, fmt.Sprintf("failed to read bundle mediaType: %v", err), ext.GetGeneration()) |
if ext.Status.InstalledBundleResource == "" { | ||
setInstalledStatusConditionUnknown(&ext.Status.Conditions, fmt.Sprintf("bundle type %s not supported currently", mediaType), ext.GetGeneration()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would only be expecting changes to the InstalledBundleResource
and the Installed
condition based on Get
-ing the existing App
.
Resolution and Installation are orthogonal. A failure computing a resolution shouldn't impact our ability to assess the installed state and vice versa.
This is similar to the point I made above about assessing Deprecation status separately.
@@ -194,6 +198,8 @@ func (r *ExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alpha1.Ext | |||
// originally Reason: ocv1alpha1.ReasonInstallationFailed | |||
ext.Status.InstalledBundleResource = "" | |||
setInstalledStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) | |||
setProgressingStatusConditionFalse(&ext.Status.Conditions, "app install failed", ext.GetGeneration()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a case where I'd say Progressing
is still True
because we encountered a failure but are going to retry.
@@ -203,11 +209,15 @@ func (r *ExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alpha1.Ext | |||
// originally Reason: ocv1alpha1.ReasonInstallationStatusUnknown | |||
ext.Status.InstalledBundleResource = "" | |||
setInstalledStatusConditionUnknown(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) | |||
setProgressingStatusConditionFalse(&ext.Status.Conditions, "app install failed", ext.GetGeneration()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here as above. A rule of thumb is: if we're going to end up returning an error from Reconcile
, that means we're going to retry, which means we're still progressing.
Status: metav1.ConditionFalse, | ||
Reason: ocv1alpha1.ReasonResolutionFailed, | ||
Reason: ocv1alpha1.ReasonPending, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree to that. ProgressingFalse
can indicate a few things:
- Installation of the bundle was successful, we are in a stable state.
- Installation of bundle failed, we are again in a stable state, but look to the install status to know more.
- We are at the Resolution step for the next reconcile probably caused by upgrade - haven't yet reached to the point of install.
// setResolvedStatusConditionUnknown sets the resolved status condition to unknown. | ||
func setResolvedStatusConditionUnknown(conditions *[]metav1.Condition, message string, generation int64) { | ||
// setProgressingStatusConditionUnknown sets the progressing status condition to unknown. | ||
func setProgressingStatusConditionUnknown(conditions *[]metav1.Condition, message string, generation int64) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've commented below on trying to avoid Unknown
for Progressing.
Reason: ocv1alpha1.ReasonUnsupportedOrInvalidBundle, | ||
Message: message, | ||
ObservedGeneration: generation, | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: We could also pass in the the reason to the method signature, such that these helpers are used to just indicate if the particular Status
is set to false or true. Its a personal preference, so whatever you think makes it easier to read the code.
ext.Status.ResolvedBundleResource = "" | ||
setResolvedStatusConditionUnknown(&ext.Status.Conditions, "extension feature is disabled", ext.GetGeneration()) | ||
setProgressingStatusConditionUnknown(&ext.Status.Conditions, "extension feature is disabled", ext.GetGeneration()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see here.
Do we need to reset the progressing condition here to unknown? Can't it just be false? Having too many combinations can also be confusing, it would be nice if we can reduce the dimensionality of Progressing
to either False or True. Especially given we have clarity on other stages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, but as I mentioned on our call, I'm also personally okay with following the existing pattern in this PR and then working in a separate followup to reorganize the code around the fact that the various condition types can be calculated and determined in isolation from each other.
ext.Status.ResolvedBundleResource = "" | ||
setResolvedStatusConditionUnknown(&ext.Status.Conditions, "kapp apis are unavailable", ext.GetGeneration()) | ||
|
||
setProgressingStatusConditionUnknown(&ext.Status.Conditions, "kapp apis are unavailable", ext.GetGeneration()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also suggest to make this False
instead of Unknown
. This bit of code that checks if the Apis exists is going to be removed anyway though. The change is in the unit test PR.
ext.Status.ResolvedBundleResource = "" | ||
setResolvedStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) | ||
setProgressingStatusConditionResolutionFailed(&ext.Status.Conditions, "resolution failed: "+err.Error(), ext.GetGeneration()) | ||
setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as resolution failed", ext.GetGeneration()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#671 should address it
ext.Status.InstalledBundleResource = "" | ||
setInstalledStatusConditionUnknown(&ext.Status.Conditions, fmt.Sprintf("bundle type %s not supported currently", mediaType), ext.GetGeneration()) | ||
if ext.Status.InstalledBundleResource == "" { | ||
setInstalledStatusConditionUnknown(&ext.Status.Conditions, fmt.Sprintf("bundle type %s not supported currently", mediaType), ext.GetGeneration()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm starting to re-think based on my previous comment of avoiding Unknown
in Progressing
. Can we also make this to False
?
Resolution hasn't begun, spec is invalid, and no instance of bundle exists on the cluster. The above does give us some confidence to set Install to false.
setInstalledStatusConditionUnknown(&ext.Status.Conditions, "install status unknown", ext.Generation) | ||
// mapAppStatusToCondition maps the reconciling/deleting App conditions to the installed/deleting conditions on the Extension. | ||
func MapAppStatusToCondition(existingApp *kappctrlv1alpha1.App, ext *ocv1alpha1.Extension) { | ||
// Note: App.Status.Inspect errors are never surfaced to App conditions, so are currently ignored when determining App status. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iirc, also Status.Inspect
appears if the knob to inspect resources is set to true in the spec. We would have to probably cover this aspect better with the health checks.
for _, cond := range orderedAppStatuses { | ||
if c := findStatusCondition(existingApp.Status.GenericStatus.Conditions, cond); c != nil && c.Status == corev1.ConditionTrue { | ||
if len(message) == 0 { | ||
message = c.Message | ||
} | ||
appStatusMapFn[cond](&ext.Status.Conditions, message, ext.Generation) | ||
return | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of looping through these orderedAppStatuses
and then doing a loop through the conditions to find them, could we just loop through all the App conditions, check if there is a status mapping function, and if there is one call it?
139a87d
to
fe5b3c5
Compare
6deccc5
to
30709da
Compare
Signed-off-by: Ankita Thomas <ankithom@redhat.com>
30709da
to
781f915
Compare
781f915
to
55085aa
Compare
55085aa
to
0e1abc3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job @ankitathomas! Just one nit. Once you decide on that, let's merge it!
apimeta.SetStatusCondition(conditions, metav1.Condition{ | ||
Type: ocv1alpha1.TypeProgressing, | ||
Status: metav1.ConditionFalse, | ||
Reason: ocv1alpha1.ReasonSuccess, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: something like ReasonReachedDesiredIntent
seems more apt for the False
status of the progressing condition. WDYT?
// Populate the deprecation status using the resolved bundle | ||
SetDeprecationStatusInExtension(ext, bundle) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Based on the current state of the logic in determining deprecation status, and without increasing scope to include modifications to it, I think this shouldn't be called until installation has succeeded.
There is some nuance where we theoretically could set the package and/or channel deprecation status after resolution but IMO any changes to the existing deprecation logic is out of scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me. I had one nit but I'm OK merging this without that being addressed since we are also shortly planning on doing a "soft removal" of the Extension API while we focus on the ClusterExtension API
Signed-off-by: Ankita Thomas <ankithom@redhat.com>
ce3a005
90fc6ba
to
ba6ec56
Compare
Signed-off-by: Ankita Thomas <ankithom@redhat.com>
ba6ec56
to
27a6d32
Compare
appStatusMapFn := map[kappctrlv1alpha1.ConditionType]func(*[]metav1.Condition, string, int64){ | ||
kappctrlv1alpha1.Deleting: setProgressingStatusConditionProgressing, | ||
kappctrlv1alpha1.Reconciling: setProgressingStatusConditionProgressing, | ||
kappctrlv1alpha1.DeleteFailed: setProgressingStatusConditionFailed, | ||
kappctrlv1alpha1.ReconcileFailed: setProgressingStatusConditionFailed, | ||
kappctrlv1alpha1.ReconcileSucceeded: setProgressingStatusConditionSuccess, | ||
} | ||
for cond := range appStatusMapFn { | ||
if c := findStatusCondition(existingApp.Status.GenericStatus.Conditions, cond); c != nil && c.Status == corev1.ConditionTrue { | ||
if len(message) == 0 { | ||
message = c.Message | ||
} | ||
appStatusMapFn[cond](&ext.Status.Conditions, fmt.Sprintf("App %s: %s", c.Type, message), ext.Generation) | ||
return | ||
} | ||
} | ||
if len(message) == 0 { | ||
message = "waiting for app" | ||
} | ||
setProgressingStatusConditionProgressing(&ext.Status.Conditions, message, ext.Generation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a huge fan of looping through all the status conditions 5 times but since we are moving away from building on top of the App CR and this is an optimization that could be made in the future I'm fine with it for now
Description
Decouple the extension status conditions and introduce a new Progressing condition.
See #655
The installed condition now indicates whether the last installed App was successfully applied.
The progressing condition indicates whether resolution, installation, upgrade or app reconciliation is in flight, with the reason and message on the condition providing more information.
Reviewer Checklist